test/e2e/e2e_test.go (2)

726-727: Remove unused constants.

clusterSecretStoreFile and externalSecretFile are declared but never used. The actual file paths are defined locally in the test function (lines 790-792).

 	Context("Vault Secret Manager", Label("Platform:Vault"), func() {
 		const (
-			clusterSecretStoreFile = "testdata/vault/secret_store.yaml"
-			externalSecretFile     = "testdata/vault/external_secret.yaml"
 			vaultSecretName        = "foo"
 			vaultSecretKey         = "my-value"
 			vaultSecretValue       = "bar"
 		)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/e2e_test.go` around lines 726 - 727, Remove the unused constants
clusterSecretStoreFile and externalSecretFile from the top-level declarations in
the test file; they are never referenced and duplicate the local paths that are
used inside the test function (where the actual file paths are defined around
lines ~790-792). Locate and delete the constant definitions named
clusterSecretStoreFile and externalSecretFile so only the locally scoped
variables remain.

---

1031-1037: Shell command construction may break with special characters.

The fmt.Sprintf constructs a shell command with direct string interpolation. If secretname, key, or value contain shell metacharacters (quotes, backticks, $), the command could fail or behave unexpectedly.

For test code with controlled inputs this is low risk, but consider using separate arguments to vault kv put to avoid shell interpretation issues.

Alternative: pass key-value without shell quoting

 	cmd := exec.Command(
-		"oc", "exec", "-n", vaultNamespace, podName, "--", "sh", "-c",
-		fmt.Sprintf(
-			"vault kv put secret/%s %s=\"%s\"",
-			secretname, key, value,
-		),
+		"oc", "exec", "-n", vaultNamespace, podName, "--",
+		"vault", "kv", "put", fmt.Sprintf("secret/%s", secretname),
+		fmt.Sprintf("%s=%s", key, value),
 	)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/e2e_test.go` around lines 1031 - 1037, The test builds a shell
command via fmt.Sprintf and sh -c which allows shell metacharacters in
secretname/key/value to break behavior; update the exec.Command invocation (the
cmd := exec.Command(...) that currently uses "sh", "-c" and fmt.Sprintf("vault
kv put secret/%s %s=\"%s\"")) to call vault kv put with separate arguments
instead of a shell string—e.g., remove "sh", "-c" and pass "vault","kv","put",
fmt.Sprintf("secret/%s", secretname), fmt.Sprintf("%s=%s", key, value) (or
construct the key=value arg safely) so that exec.Command handles argument
quoting and no shell interpolation occurs.

Makefile (1)

219-230: Inconsistencies with existing test-e2e target may cause issues.

The new e2e-test-vault target differs from test-e2e in several ways that could cause problems:

1. Missing GOFLAGS= override — line 182 sets this for test-e2e to avoid conflicts with go.work, but e2e-test-vault is not included
2. Missing @ prefix to suppress command echo
3. Missing -ginkgo.trace flag
4. Different working directory approach (./test/e2e vs -C $(PROJECT_ROOT)/test ./e2e)

Suggested fix for consistency

-	
-.PHONY: e2e-test-vault  # Run the e2e tests against a Kind k8s instance that is spun up.
+
+# Targets that need Go workspace mode (CI sets GOFLAGS=-mod=vendor which conflicts with go.work)
+fmt vet test test-unit test-e2e e2e-test-vault run update-vendor update-dep: GOFLAGS=
+
+.PHONY: e2e-test-vault
 e2e-test-vault:
-	go test \
+	`@go` test -C $(PROJECT_ROOT)/test \
 	-timeout $(E2E_TIMEOUT) \
 	-count 1 \
 	-v \
 	-p 1 \
 	-tags e2e \
-	./test/e2e \
+	./e2e \
 	-ginkgo.v \
+	-ginkgo.trace \
 	-ginkgo.show-node-events \
 	-ginkgo.label-filter="$(E2E_GINKGO_LABEL_FILTER)"

Note: Update line 182 to include e2e-test-vault in the GOFLAGS override list.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Makefile` around lines 219 - 230, The new e2e-test-vault Make target is
inconsistent with test-e2e; update e2e-test-vault to match: ensure it's covered
by the GOFLAGS override (add e2e-test-vault to the existing GOFLAGS= override
that prevents go.work conflicts), prefix the go test invocation with @ to
suppress echo, add the -ginkgo.trace flag to the ginkgo flags, and run the tests
from the same directory style as test-e2e (use -C $(PROJECT_ROOT)/test ./e2e
rather than ./test/e2e) while keeping the same flags (-timeout $(E2E_TIMEOUT)
-count 1 -v -p 1 -tags e2e -ginkgo.v -ginkgo.show-node-events
-ginkgo.label-filter="$(E2E_GINKGO_LABEL_FILTER)").

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/e2e/e2e_test.go (3)

725-731: Remove unused constants.

clusterSecretStoreFile and externalSecretFile are declared but never used. The test instead defines local variables vaultSecretStoreFile and vaultExternalSecretFile at lines 784-785.

Suggested fix

 	Context("Vault Secret Manager", Label("Platform:Vault"), func() {
 		const (
-			clusterSecretStoreFile = "testdata/vault/secret_store.yaml"
-			externalSecretFile     = "testdata/vault/external_secret.yaml"
-			vaultSecretName        = "foo"
-			vaultSecretKey         = "my-value"
-			vaultSecretValue       = "bar"
+			vaultSecretName  = "foo"
+			vaultSecretKey   = "my-value"
+			vaultSecretValue = "bar"
 		)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/e2e_test.go` around lines 725 - 731, Remove the unused constants
clusterSecretStoreFile and externalSecretFile from the constants block (they are
shadowed by local variables vaultSecretStoreFile and vaultExternalSecretFile
later in the test), leaving only the actually used constants vaultSecretName,
vaultSecretKey, and vaultSecretValue; update the constants declaration near the
top of the test in e2e_test.go so there are no unused variables left.

---

874-874: Remove unused loader parameter.

The loader parameter is passed to applyVault but never used within the function.

Suggested fix

-func applyVault(ctx context.Context, loader utils.DynamicResourceLoader) error {
+func applyVault(ctx context.Context) error {

And update the call site at line 735:

-		Expect(applyVault(ctx, loader)).To(Succeed())
+		Expect(applyVault(ctx)).To(Succeed())

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/e2e_test.go` at line 874, The applyVault function signature includes
an unused parameter loader; remove the unused parameter from the applyVault
declaration (change func applyVault(ctx context.Context, loader
utils.DynamicResourceLoader) to func applyVault(ctx context.Context)) and update
all call sites that pass a loader to call applyVault with only the ctx argument
(i.e., remove the extra argument where applyVault is invoked) so the function
and its callers remain consistent.

---

1004-1014: Consider more granular error handling in enableKVEngine.

The || true at line 1007 masks all errors, including authentication failures or Vault unavailability. While this is intentional for idempotent KV engine enabling, consider checking only for the "already enabled" error.

Alternative approach

 	cmd := exec.Command(
 		"oc", "exec", "-n", vaultNamespace, podName, "--", "sh", "-c",
 		fmt.Sprintf(
-			"vault status && vault login %s && vault secrets enable -path=secret kv-v2 || true",
+			"vault status && vault login %s && (vault secrets enable -path=secret kv-v2 2>&1 || echo 'KV engine may already be enabled')",
 			token,
 		),
 	)

Alternatively, check the exit code explicitly and only ignore the "path is already in use" error.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/e2e_test.go` around lines 1004 - 1014, The current command uses "||
true" which masks all errors; in the enableKVEngine logic replace the blanket
ignore with targeted handling: run the same exec.Command via utils.Run (the
existing cmd, podName, vaultNamespace, token) but if it returns an error inspect
the output/error string or exit code and only suppress the error when it
indicates the KV engine is already enabled (e.g., message like "path is already
in use" or equivalent); for any other error (authentication failure, vault
unavailable, etc.) propagate/return the error so failures are not silently
ignored.

test/e2e/testdata/vault/vault.yaml (1)

53-84: Consider adding resource requests/limits.

The container lacks resource requests and limits. While acceptable for e2e tests, defining them can prevent resource contention and improve scheduling predictability in shared test clusters.

💡 Optional: Add resource constraints

           ports:
             - containerPort: 8200          
           securityContext:            
             allowPrivilegeEscalation: false            
             capabilities:
               drop:
                 - ALL              
+          resources:
+            requests:
+              cpu: 100m
+              memory: 256Mi
+            limits:
+              cpu: 500m
+              memory: 512Mi
           volumeMounts:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/testdata/vault/vault.yaml` around lines 53 - 84, The vault container
spec (container name "vault") is missing resources; add a resources block under
the container to set requests and limits (e.g., resources.requests.cpu,
resources.requests.memory and resources.limits.cpu, resources.limits.memory) so
the "vault" container has both minimal guaranteed resources and caps; update the
same container entry that contains command/args/readinessProbe/volumeMounts to
include these fields with appropriate values for the e2e environment.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/e2e/testdata/vault/vault.yaml (1)

1-5: ⚠️ Potential issue | 🟡 Minor

Fix newline character format.

Static analysis indicates the file uses incorrect line endings (CRLF). Convert to Unix-style line endings (\n) for CI compatibility.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/testdata/vault/vault.yaml` around lines 1 - 5, The YAML file
defining the Namespace (look for the block with "apiVersion: v1", "kind:
Namespace", and "name: vault-test") uses CRLF line endings and must be converted
to Unix-style LF; open that vault.yaml, normalize line endings to "\n" (e.g.,
use your editor's EOL conversion, run dos2unix, or git's core.autocrlf
settings), verify there are no trailing CR (\r) characters, and recommit the
file.

test/e2e/testdata/vault/vault.yaml (1)

1-115: ⚠️ Potential issue | 🟡 Minor

Re-save this manifest with LF line endings.

YAMLlint is still reporting CRLF here, so CI will keep failing until the file is saved with Unix newlines.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/testdata/vault/vault.yaml` around lines 1 - 115, The manifest uses
CRLF endings; re-save vault.yaml with Unix LF line endings so YAML lint passes
(affects resources like the Namespace named "vault-test",
Deployment/ServiceAccount/Deployment named "vault", and ConfigMap
"vault-config"); open the file in your editor or run a tool (e.g., convert line
endings or dos2unix) to normalize to LF, commit the updated file, and ensure
your local git autocrlf setting won't reintroduce CRLF.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/e2e/e2e_test.go (1)

994-997: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Match custom NetworkPolicy entries by the full list key.

networkPolicies is keyed by both name and componentName; checking only Name can skip adding the CoreController policy if another component already has the same policy name.

Suggested fix

 				for _, np := range currentCR.Spec.ControllerConfig.NetworkPolicies {
-					if np.Name == testPolicyName {
+					if np.Name == testPolicyName && np.ComponentName == operatorv1alpha1.CoreController {
 						return nil
 					}
 				}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/e2e_test.go` around lines 994 - 997, The NetworkPolicy lookup in
currentCR.Spec.ControllerConfig.NetworkPolicies only checks np.Name, which can
incorrectly treat different component entries as the same policy. Update the
matching logic in the NetworkPolicies loop to use the full key for custom
entries, including both Name and ComponentName, so the CoreController policy is
added when a different component has the same policy name.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes